Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#190-FetchInteger: Fix #492

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

#190-FetchInteger: Fix #492

wants to merge 2 commits into from

Conversation

artiodev
Copy link

@artiodev artiodev commented Aug 8, 2024

#190
Handles int and bool types when returned as string map value.

@yxxhero yxxhero linked an issue Aug 8, 2024 that may be closed by this pull request
Fetch integer value from JSON #190
Open
@yxxhero
Copy link
Member

yxxhero commented Aug 8, 2024

@artiodev thanks so much. could you add some tests?

Gianluca Artioli added 2 commits August 13, 2024 00:27
helmfile#190-FetchInteger: Fix
bf5fe1e 
Signed-off-by: Gianluca Artioli <gartioli@doxee.com>
helmfile#190-FetchInteger: TestCase
f22b744 
Signed-off-by: Gianluca Artioli <gartioli@doxee.com>
@artiodev
Copy link
Author

@yxxhero Done

@yxxhero
Copy link
Member

yxxhero commented Aug 12, 2024

@mumoshu WDYT?

mumoshu
mumoshu reviewed Aug 13, 2024
View reviewed changes
vals_test.go
Comment on lines +130 to +131
ResultBool: "true"
ResultInteger: "1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@artiodev Thanks a bunch for the pull request!
This looks well aligned with vals' design, where it wants any fetched values to be rendered as strings, so that the values before(ref+...) and after the vals rendering are both strings. It's great in that regard.

But, is this what you originally wanted in #190?

Here, I was wondering if you wanted:

Suggested change
ResultBool: "true"
ResultInteger: "1"
ResultBool: true
ResultInteger: 1

rather than the string representations of the original values.

If necessary, we could support both options, perhaps via feature toggling envvars and or ref URL params. Just wanted to confirm your goal and intention to start!

Thanks in advance for your cooperation 🙏

@artiodev
Copy link
Author

Hello @mumoshu,
You're right! This is the result I want, but I think is quite difficult to have.
ResultBool: "true" ResultInteger: "1" ResultBool: true ResultInteger: 1
Maybe it could be change the Lookup function using Generics type?

However at the moment this solution could be enough for me.

Thanks!

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 16, 2024

Hey @artiodev! Thanks for confirming!

From vals' design perspective, I believe that both work if each is enabled only in an opt-in manner.

Would you like it, if it looked like this:

fetched value ref URL param result
1 error (because vals expects string by default)
1 as=int 1
1 as=string "1"
true error (because vals expects string by default)
true as=int error (because we won't define true->int conversion or vice versa)
true as=bool true
true as=string "true"

We initially implement as=int and as=bool only because I suppose, in many cases, you don't need to turn anything into strings with vals. Put another way, we won't have as=string until it's absolutely necessary.

Does that make sense to you?

cc/ @yxxhero

@yxxhero
Copy link
Member

yxxhero commented Aug 20, 2024

Hey @artiodev! Thanks for confirming!

From vals' design perspective, I believe that both work if each is enabled only in an opt-in manner.

Would you like it, if it looked like this:

fetched value ref URL param result
1 error (because vals expects string by default)
1 as=int 1
1 as=string "1"
true error (because vals expects string by default)
true as=int error (because we won't define true->int conversion or vice versa)
true as=bool true
true as=string "true"
We initially implement as=int and as=bool only because I suppose, in many cases, you don't need to turn anything into strings with vals. Put another way, we won't have as=string until it's absolutely necessary.

Does that make sense to you?

cc/ @yxxhero

yeah. it's better.

@artiodev
Copy link
Author

artiodev commented Aug 20, 2024 •
edited
Loading

yeah, This solution is even better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mumoshu mumoshu mumoshu left review comments

@itscaro itscaro Awaiting requested review from itscaro itscaro is a code owner

@yxxhero yxxhero Awaiting requested review from yxxhero yxxhero is a code owner

@xiaomudk xiaomudk Awaiting requested review from xiaomudk xiaomudk is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fetch integer value from JSON
3 participants
@artiodev @yxxhero @mumoshu